[SYCL] Implement atomic_memory_scope_capabilities device query for OpenCL and Level Zero #8595

maarquitos14 · 2023-03-09T16:11:04Z

Adds support to query devices for atomic_memory_scope_capabilities. The backends supported are OpenCL and Level Zero. For the rest of backends, it has been left unsupported.

… and Level Zero backends. Signed-off-by: Maronas, Marcos <[email protected]>

Signed-off-by: Maronas, Marcos <[email protected]>

…memory-scope

Signed-off-by: Maronas, Marcos <[email protected]>

sycl/include/sycl/detail/pi.h

sycl/plugins/opencl/pi_opencl.cpp

AlexeySachkov · 2023-03-10T14:21:21Z

sycl/plugins/opencl/pi_opencl.cpp

+      // Because scopes are hierarchical, wider scopes support all narrower
+      // scopes. SUB_GROUP and WORK_ITEM were already included in the
+      // initialization, since WORK_GROUP is mandated minimum capality.
+      if (devCapabilities && PI_DEVICE_ATOMIC_SCOPE_DEVICE) {
+        result |= PI_MEMORY_SCOPE_DEVICE;
+      }


Oh, I suppose that's the answer to my question above about default value. Do you have a pointer to spec section which describes the hierarchical nature of those scopes?

This comes from OpenCL Spec: https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_API.html#CL_DEVICE_ATOMIC_MEMORY_CAPABILITIES

However, you can see there that WORK_ITEM is actually an exception in OpenCL. I had an offline discussion with @gmlueck, @bashbaug and @Pennycook about this, and the conclusion is that SYCL should always return WORK_ITEM. I think there will be an update in the spec to detail this.

Got it, thanks! Perhaps it would be better to put that link to OCL spec directly into sources next to the comment

I thought about that, but the OCL spec explicitly says WORK_ITEM is an exception, so that might be confusing because we are not treating it as an exception. When SYCL spec is updated, we can obviously add a ref to that next to the comment. For now, I'm not sure if referencing OCL spec is helpful or more confusing. What do you think?

Technically, this code is within OpenCL plugin and therefore I think it shouldn't be too confusing to leave links to OpenCL spec in there.

Discussed this offline. There is a corner case with WORK_ITEM scope being defined differently between SYCL and OpenCL, which is a reason for ignoring this OpenCL exception.

I think that it would still make sense to leave an OCL spec link, but we should have a comment about why we ignore that work-item exception. If we already have a link to SYCL spec clarification PR/issue, then even better, we can link it there as well.

I extended the current comment with further explanation and a link to OCL spec. Please, let me know if this is enough.

sycl/plugins/opencl/pi_opencl.cpp

Signed-off-by: Maronas, Marcos <[email protected]>

AlexeySachkov

Overall LGTM

opencl/CMakeLists.txt

AlexeySachkov · 2023-03-15T10:03:35Z

sycl/include/sycl/detail/cl.h

@@ -11,7 +11,7 @@
 // Suppress a compiler message about undefined CL_TARGET_OPENCL_VERSION
 // and define all symbols up to OpenCL 2.2


Suggested change

// and define all symbols up to OpenCL 2.2

// and define all symbols up to OpenCL 3.0

AlexeySachkov · 2023-03-15T10:06:32Z

sycl/plugins/opencl/pi_opencl.cpp

+      if (paramValueSizeRet)
+        *paramValueSizeRet = sizeof(result);
+    }


Suggested change

if (paramValueSizeRet)

*paramValueSizeRet = sizeof(result);

}

}

if (paramValueSizeRet)

*paramValueSizeRet = sizeof(result);

paramValueSizeRet should be filled even if paramValue is nullptr.
That is needed to support (usually) initial query:

piDeviceInfoGet(device, paramName, nullptr, 0, &size_ret); // now allocate size_ret bytes and query result

AlexeySachkov · 2023-03-15T10:06:53Z

sycl/plugins/opencl/pi_opencl.cpp

@@ -1787,7 +1842,7 @@ pi_result piextKernelGetNativeHandle(pi_kernel kernel,
 // Windows: dynamically loaded plugins might have been unloaded already
 // when this is called. Sycl RT holds onto the PI plugin so it can be
 // called safely. But this is not transitive. If the PI plugin in turn
-// dynamically loaded a different DLL, that may have been unloaded. 
+// dynamically loaded a different DLL, that may have been unloaded.


Seems like an unrelated change, better to avoid

AlexeySachkov · 2023-03-15T10:09:43Z

sycl/plugins/opencl/pi_opencl.cpp

+      // Because scopes are hierarchical, wider scopes support all narrower
+      // scopes. SUB_GROUP and WORK_ITEM were already included in the
+      // initialization, since WORK_GROUP is mandated minimum capality.
+      if (devCapabilities && PI_DEVICE_ATOMIC_SCOPE_DEVICE) {
+        result |= PI_MEMORY_SCOPE_DEVICE;
+      }


Technically, this code is within OpenCL plugin and therefore I think it shouldn't be too confusing to leave links to OpenCL spec in there.

AlexeySachkov · 2023-03-16T12:24:40Z

@intel/dpcpp-esimd-reviewers, could you please take a look at esimd_emulator part of the PR? It is fairly simple

sarnex

esimd emu lgtm

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 · 2023-03-16T20:30:05Z

Friendly ping @intel/llvm-reviewers-runtime @intel/dpcpp-l0-pi-reviewers

smaslov-intel · 2023-03-17T01:01:19Z

sycl/plugins/unified_runtime/ur/adapters/level_zero/ur_level_zero.cpp

+  case UR_DEVICE_INFO_ATOMIC_MEMORY_SCOPE_CAPABILITIES: {
+    // There are no explicit restrictions in L0 programming guide, so assume all
+    // are supported
+    pi_memory_scope_capabilities result =
+        PI_MEMORY_SCOPE_WORK_ITEM | PI_MEMORY_SCOPE_SUB_GROUP |
+        PI_MEMORY_SCOPE_WORK_GROUP | PI_MEMORY_SCOPE_DEVICE |
+        PI_MEMORY_SCOPE_SYSTEM;
+
+    return ReturnValue(result);
+  }


@jandres742 : please review this

assumption is correct for L0 GPU driver, based on what we return already in OpenCL GPU driver.

smaslov-intel

LGTM

jandres742

+1

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 · 2023-03-21T09:05:17Z

@intel/llvm-reviewers-runtime assigned reviewer is on holiday, could you please take a look?

steffenlarsen

LGTM! 🚀

Signed-off-by: Maronas, Marcos <[email protected]>

bader · 2023-03-22T22:48:19Z

@maarquitos14, sycl/unittests/SYCL2020/AtomicMemoryScopeCapabilities.cpp fails on Windows. Please, fix ASAP.

…enCL and Level Zero (intel#8595) Adds support to query devices for `atomic_memory_scope_capabilities`. The backends supported are OpenCL and Level Zero. For the rest of backends, it has been left unsupported. --------- Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 added 2 commits March 9, 2023 08:07

Implements atomic memory scope capabilities device queries for OpenCL…

40e5bc2

… and Level Zero backends. Signed-off-by: Maronas, Marcos <[email protected]>

Adds test for atomic memory scope capabilities device queries.

db64e3c

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 requested review from a team as code owners March 9, 2023 16:11

maarquitos14 requested a review from dm-vodopyanov March 9, 2023 16:11

maarquitos14 temporarily deployed to aws March 9, 2023 16:43 — with GitHub Actions Inactive

Merge remote-tracking branch 'intel/origin/sycl' into maronas/atomic-…

43b0bf7

…memory-scope

maarquitos14 temporarily deployed to aws March 10, 2023 10:03 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws March 10, 2023 10:31 — with GitHub Actions Inactive

SYCL should always return memory_scope::work_item.

56badc0

Signed-off-by: Maronas, Marcos <[email protected]>

AlexeySachkov reviewed Mar 10, 2023

View reviewed changes

maarquitos14 temporarily deployed to aws March 10, 2023 14:36 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws March 10, 2023 15:16 — with GitHub Actions Inactive

andylshort mentioned this pull request Mar 10, 2023

[SYCL] atomic_memory_order_capabilities query for device and context #8517

Merged

Addressing code review concerns.

41b5cc1

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 temporarily deployed to aws March 13, 2023 17:05 — with GitHub Actions Inactive

Fixing compilation error.

8392d5b

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 temporarily deployed to aws March 13, 2023 18:49 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws March 13, 2023 19:47 — with GitHub Actions Inactive

AlexeySachkov approved these changes Mar 15, 2023

View reviewed changes

sarnex approved these changes Mar 16, 2023

View reviewed changes

Addresses code review comments.

8bacef1

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 requested a review from a team as a code owner March 16, 2023 16:01

Reverting unrelated change.

3c05a25

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 temporarily deployed to aws March 16, 2023 16:31 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws March 16, 2023 17:01 — with GitHub Actions Inactive

smaslov-intel reviewed Mar 17, 2023

View reviewed changes

smaslov-intel approved these changes Mar 17, 2023

View reviewed changes

jandres742 reviewed Mar 17, 2023

View reviewed changes

Clarifies comment using new reference.

69d901b

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 temporarily deployed to aws March 17, 2023 12:50 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws March 17, 2023 13:36 — with GitHub Actions Inactive

Extends comment.

1005e80

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 temporarily deployed to aws March 17, 2023 15:57 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws March 17, 2023 16:26 — with GitHub Actions Inactive

steffenlarsen approved these changes Mar 21, 2023

View reviewed changes

Addresses code review comments.

f7aa8ee

Signed-off-by: Maronas, Marcos <[email protected]>

maarquitos14 temporarily deployed to aws March 22, 2023 16:32 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws March 22, 2023 17:52 — with GitHub Actions Inactive

bader changed the title ~~[SYCL] Implements atomic_memory_scope_capabilities device query for OpenCL and Level Zero~~ [SYCL] Implement atomic_memory_scope_capabilities device query for OpenCL and Level Zero Mar 22, 2023

bader merged commit 2816c14 into intel:sycl Mar 22, 2023

		@@ -11,7 +11,7 @@
		// Suppress a compiler message about undefined CL_TARGET_OPENCL_VERSION
		// and define all symbols up to OpenCL 2.2

	// and define all symbols up to OpenCL 2.2
	// and define all symbols up to OpenCL 3.0

[SYCL] Implement atomic_memory_scope_capabilities device query for OpenCL and Level Zero #8595

[SYCL] Implement atomic_memory_scope_capabilities device query for OpenCL and Level Zero #8595

Uh oh!

Conversation

maarquitos14 commented Mar 9, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maarquitos14 Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexeySachkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sarnex left a comment

Choose a reason for hiding this comment

Uh oh!

maarquitos14 commented Mar 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

jandres742 left a comment

Choose a reason for hiding this comment

Uh oh!

maarquitos14 commented Mar 21, 2023

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

bader commented Mar 22, 2023

Uh oh!

Uh oh!

maarquitos14 Mar 16, 2023 •

edited

Loading

AlexeySachkov commented Mar 16, 2023 •

edited

Loading